Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removing HTTP request in _PropertyMixin.properties. #688

Merged
merged 1 commit into from
Mar 13, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Mar 4, 2015

NOTE 1: Has #683 as diffbase.

NOTE 2: There are many places here where the tests just give up and call _reload_properties(). This should not be. We should be clear on which methods should be just returning local data and which should make an HTTP request (e.g. update to Bucket.get_logging() here). (I also partially point out this type of problem in #683)

Also removing dead code:

  • _PropertyMixin.CUSTOM_PROPERTY_ACCESSORS (and subclasses use of this)
  • _PropertyMixin._get_property (only consumer of CUSTOM_PROPERTY_ACCESSORS)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 4, 2015
@dhermes dhermes added do not merge Indicates a pull request not ready for merge, due to either quality or timing. api: storage Issues related to the Cloud Storage API. labels Mar 4, 2015
@dhermes dhermes force-pushed the storage-properties-no-http branch from 983ce7f to 494b838 Compare March 5, 2015 18:38
@dhermes
Copy link
Contributor Author

dhermes commented Mar 5, 2015

@tseaver Rebased on top of #683 after merge. PTAL.

This addresses the second of my 4 concerns.

@dhermes dhermes force-pushed the storage-properties-no-http branch from 494b838 to 51f70c6 Compare March 9, 2015 18:08
@dhermes dhermes removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 9, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Mar 9, 2015

@tseaver I just took out all the XXX discussion points from the commit and so removed the don't merge label.

There were six places (in the old commit) where I was unsure of what was happening and wanted to discuss:

  • Calling _reload_properties before bucket.get_cors()
  • Calling _reload_properties before bucket.get_lifecycle()
  • Needing 3 mocked HTTP responses to call bucket.get_logging() before and after bucket.enable_logging(...) (2 different places)
  • Same as above, but for calling bucket.disable_logging()
  • Calling _reload_properties before using bucket.versioning_enabled getter

@tseaver
Copy link
Contributor

tseaver commented Mar 9, 2015

I don't have a problem with the implementation, but I'm not sure we have addressed what to do with the "don't make me think" usability issue introduced by removing "eager" loads.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 9, 2015

@tseaver Yeah for sure we've got to get a better handle on that.

But if people typically get a bucket via storage.get_bucket(bucket_name) then properties will already be populated.

Those crafting one by hand via Bucket(bucket_name) may fall into the class of more sophisticated users.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 9, 2015

@tseaver After #683 the implementation is kind of in flux and moving towards a new equilibrium.

I am more worried about being consistent in methods like get_logging() that are essentially just getters.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 9, 2015

@tseaver I restarted the Travis build and it kickstarted coveralls into action

@tseaver
Copy link
Contributor

tseaver commented Mar 9, 2015

For the case where missing "eager" messages would hurt: should getters raise an exception (with a message indicating "call load()"?)

@dhermes
Copy link
Contributor Author

dhermes commented Mar 9, 2015

What do you mean by eager messages? Also, should this discussion block this PR? (I see this PR as a tiny piece of moving towards whatever monolith change #632 represents.)

Yes, I think we should raise in some situations. Or provide an load_properties=False boolean in the constructor. Or some other such thing.

When we get the same level of implicit (and lazy-loading) support that we have in datastore, we could also make eager vs. declarative a setting that people could turn on.

@tseaver
Copy link
Contributor

tseaver commented Mar 9, 2015

I meant to ask whether attempts to use not-yet-fetched properties should fail? E.g., Bucket.get_cors() will return an empty list if _load_properties has not yet been called, even though the server-side might actually have values; should it be an error?

Essentially, this is the same as the "testing" problem you listed above (where you have to call bucket._reload_properties()): the PR deletes the "eager" tests (which called it implicitly), but leaves those paths in limbo.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 9, 2015

@tseaver Yes I think we should just add a dirty boolean (probably needs a better name) and a timestamp of "last retrieved" / "last synced".

But for things like get_cors(), maybe we want them to make the HTTP request (i.e. not behave like a getter, but like an HTTP request).

@tseaver
Copy link
Contributor

tseaver commented Mar 11, 2015

We need to decide then if we are going to keep any "non-dirty" state client side. Having to make multiple HTTP requests to fetch different bits of the state is not exactly performant, compared to the "lazy fetch" we have before this PR series.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 11, 2015

We just need a declarative get / load / reload so that users can populate the contents of _properties

Also removing dead code:
- _PropertyMixin.CUSTOM_PROPERTY_ACCESSORS (and subclasses use of this)
- _PropertyMixin._get_property (only consumer of CUSTOM_PROPERTY_ACCESSORS)
@dhermes
Copy link
Contributor Author

dhermes commented Mar 11, 2015

@tseaver I just rebased (sorry I didn't realize #693 made this PR un-mergable).

Shall we put these concerns in #632 as points to address? We certainly need a "lazy" / "auto" story, but it should be closer to the surface of user interaction and should happen at well-defined times (i.e. in constructor given optional argument).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 706f7ad on dhermes:storage-properties-no-http into 83f92cc on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Mar 13, 2015

I guess we can move forward here. Trying to re-wire the whole API from the inside out is problematic: I'm pretty sure we should be defining the "desired" stories someplace, so that we know when we have arrived.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 13, 2015

@tseaver That's what #632 is for. High-level concepts we want to be true to. (It currently says nothing about an "eager" mode.)

As far as "inside-out", at one point I thought about just starting a brand new module and just porting features over, but decided against it.

dhermes added a commit that referenced this pull request Mar 13, 2015
Removing HTTP request in _PropertyMixin.properties.
@dhermes dhermes merged commit 982dd56 into googleapis:master Mar 13, 2015
@dhermes dhermes deleted the storage-properties-no-http branch March 13, 2015 16:40
@tseaver
Copy link
Contributor

tseaver commented Mar 13, 2015

I'd rather have "stories" which outlined what the developer experience actually looks like (vs. the "principles" bit in #632). Our current docs are missing this kind of "narrative" focus, which I feel to be a big deficiency: putting that off in another, tech-writer-maintained place (that doesn't exist yet) leaves the developer side unfocused.

@dhermes
Copy link
Contributor Author

dhermes commented Mar 13, 2015

You can open an issue similar to #632 with our desired dev exp for storage in the meantime and then we can align with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants